Repository navigation
fix(core): preview playback moves every frame between an audio track's time updates - #7373
Conversation
Edit accuracy: accurate 267 (base branch 267), smooth 102 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (1)
|
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head 2a7705d7
The clock now runs up to one currentTime step ahead of the element it follows. syncRuntimeMedia still rate-steers that same element against the clock, so on the coarse-step machines this PR targets, the followed audio track is sped up and the film runs fast. Details inline.
Scope
Read clock.ts/clock.test.ts end to end, plus the paths that consume the clock: media.ts drift/steer (L660–790), mediaSteer.ts, and the per-frame syncMediaForCurrentState in init.ts. Checked the fails without the fix: claim: with main's clock.ts, the new test fails (30 ms steps: ticks a frame is held: expected 3 to be less than or equal to 1). With the PR, clock.test.ts and mediaSteer.test.ts pass (64/64). No earlier reviews. CI not read: this identity can't read check-runs, and mergeStateStatus is BLOCKED.
| } | ||
| noteStep(last, time); | ||
| const run = last.from + ((nowMs - last.fromMs) / 1000) * this._rate; | ||
| const held = Math.min(Math.max(run, time), time + furthestAhead(last)); |
There was a problem hiding this comment.
blocker: this clock now runs up to one step ahead of the element it follows, but syncRuntimeMedia still rate-steers that element against the clock. Each frame, media.ts:682 computes offset = relTime − el.currentTime, which is now up to one full step. When it passes MEDIA_SYNC_TOLERANCE_SECONDS (40 ms), steerMedia (mediaSteer.ts:55) sets the followed <audio> to 1.03x. That speeds up the element the band is anchored to, so the playhead speeds up with it. On main the offset is about 0, because the clock is the element's time.
To check this, I ran a throwaway vitest probe: this TransportClock plus the real steerMedia, with the element's position moving at its written playbackRate and currentTime floored to N ms steps. Over 10 s of wall time:
| steps | ticks | steered ticks | rate writes | audio / film speed |
|---|---|---|---|---|
| 45 ms | 60 Hz | 96/600 | 64 | 1.0048x |
| 50 ms | 144 Hz | 263/1440 | 398 | 1.0055x |
| 60 ms | 60 Hz | 224/600 | 241 | 1.0112x |
| 70 ms | 60 Hz | 275/600 | 223 | 1.0137x |
| 90 ms | 120 Hz | 624/1200 | 221 | 1.0156x |
Main gives 0 steered ticks and 1.0000x in every case. At 20/30 ms steps, and at 40/50 ms on 60 Hz, the PR is effectively clean, because the lead stays under 40 ms. That matches your manual table, which measured frame cadence but not the element's playbackRate. The body says steps are "30-50 ms or more on many machines", so this is the population the fix targets. They would get audible 1.0↔1.03 rate flips (up to ~40 writes/s at 144 Hz) and a film running 0.5–1.5% fast.
A possible fix: don't steer the clock's own element (clock.audioElement()) by its offset against the clock. Or give media sync the raw element time for that element. Then add a test that runs the clock and steerMedia together, so the two can't drift apart again.
There was a problem hiding this comment.
✅ Fixed in 8c55d88: clockAhead takes the lead off for the followed element only. clock.steer.test.ts fails against the old media.ts (45 ms @ 60 Hz: 69 rate writes) and passes now.
There was a problem hiding this comment.
Verdict: APPROVE · 1 resolved, 0 open, 0 new · 2a7705d → 8c55d88
Checked
With the old media.ts, the new clock.steer.test.ts fails (45 ms @ 60 Hz: 69 rate writes). On this head, clock, media and mediaSteer tests pass (12 files, 398 tests). _ahead cannot leak stale into a later sync: now() clears it, and seek, pause, halt and stop-at-end all re-read now() (or detach first) before syncing. CI not read: this identity can't read check-runs.
ad02236 to
ca4995c
Compare
There was a problem hiding this comment.
Verdict: APPROVE · 0 resolved, 0 open, 0 new · 8c55d88 → ca4995c
Rebased onto main after #7383 (Safari pre-roll revert). clock.ts, clock.test.ts and clock.steer.test.ts are byte-identical to the approved 8c55d88. The media.ts conflict resolution keeps main's single relTime = mediaSourceTimeAt(clip, …) (the pre-roll ternary is gone) with the clockAhead lead subtracted, and that is still the only relTime site. src/runtime: 122 files, 2867 tests pass locally. CI on this head is still running.
Port the adopted upstream fixes from f5bde95 to the current upstream tip: heygen-com#7375 multipart cloud upload, heygen-com#7382 minParallelChunks, heygen-com#7349 HLG header SEI, heygen-com#7369 skip unmounted compositions in lint/check, heygen-com#7381 filesystem-path media in the cloud archive, heygen-com#7383 Safari cut-point pre-roll revert, heygen-com#7373 clock steering between audio time updates, heygen-com#7384 injector test isolation, heygen-com#7378 preview seek retention, heygen-com#7390 virtual timer schedule per worker, heygen-com#7394 CDN script failure naming, heygen-com#7395 menu row radius, heygen-com#7119 decode timeout caller test, heygen-com#7397 authored tween order, heygen-com#7402 capture page clip loading, heygen-com#7412 muted video audio track at cuts, heygen-com#6150 template root variable test, heygen-com#7442 held tail seek once. Deliberate local differences: heygen-com#7067's audio-start hold is not carried into clock.ts, mountedSources comes from heygen-com#6827 as a helper only, cloud/_gen/types.ts omits the image-request types the fork does not generate, and the lint symlink test skips through symlinkOrSkip where the account cannot create links. Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
What
While an audio or video element drives the preview clock, the playhead now moves on every display frame between the element's
currentTimeupdates, instead of holding a frame and then jumping when the next update arrives.Why
TransportClock.now()returned the element'scurrentTimeas it was (clock.ts, the audio-master branch). Chromium does not movecurrentTimecontinuously: it moves in steps, about 20 ms in headless Chromium and 30-50 ms or more on many machines (the comment ininit.tsalready calls the element tier "~33ms"). A film at 30 fps on a 60 Hz display then shows one frame for three display frames and skips the next whenever a step is longer than a film frame. Viewers see judder, and the runtime's playback health counts the skipped frames as dropped, so the preview reads as choppy on films with a soundtrack even when the page has time to spare.To reproduce: play any film with an
<audio>clip in Chromium and logwindow.__player.getTime()once per animation frame. It stays the same for 2-3 frames, then jumps by a whole step.Related work
Refs the audio-master clock tiers in
refreshTransportClockSource; no overlapping PR.How
TransportClockkeeps a reading of the element it follows (_reading): its last distinct time, its last 8 steps, and where the clock's own run started.now()runs on at the play rate, and is only pulled back into a band: no earlier than the element's time, and no later than the element's time plus its largest recent step (steps over 250 ms are jumps, not steps). It never goes back: a pull below where the clock has already been holds the playhead until the element catches up.TransportClock.aheadOf(el)says how far the last read ran ahead of it, andsyncRuntimeMedia(clockAhead) takes that off the playhead for that element only. Without this, the lead looks like drift: past the 40 ms tolerancesteerMediasped the followed audio to 1.03x, and the playhead, which follows it, ran fast with it. Every other element is still synced to the playhead.Test plan
clock.test.tscurrentTimemoving in 30, 40 and 50 ms steps read every 1/60 s, no 30 fps film frame is skipped, no frame is shown for more than two display frames, no read goes back, and the clock is never more than one step ahead of the element;clock.steer.test.ts(new): the clock,syncRuntimeMediaand an audio element whosecurrentTimemoves in 45-90 ms steps, played at the rate media sync gives it, at 60, 120 and 144 Hz for 10 s: no rate writes, no seeks, and the film keeps its speed within 0.1%. Withmedia.tsnot taking the lead off, it fails (45 ms steps at 60 Hz: rate writes: expected 69 to be +0). It passes 3 runs in a row.clock.tsfrom main, "moves on every display frame between the element's time updates" fails (30 ms steps: ticks a frame is held: expected 3 to be less than or equal to 1).packages/coresrc/runtime: 122 files, 2868 tests pass; the clock files pass 3 runs in a row.<audio>tone in headless Chromium, the element'scurrentTimerounded down to 30, 40 or 50 ms to stand in for machines whose updates come that coarse (and once at Chromium's own steps). Per run: film frames skipped, frames held past two display frames, and the runtime's own playback-health readings. The rig measures frame cadence only; the element's playback rate is covered byclock.steer.test.ts. Runs where the page itself missed display frames are left out, since they drop frames whatever the clock does.5 runs per case; "clean runs" are those where the page drew every display frame (481 in 8 s), the rest left out because a busy machine dropped frames whatever the clock did. Frames counted after the first second, the runtime's own warm-up.
currentTimestepsAt 50 ms steps on main every playback-health reading is choppy; with this PR none is.
Size
One method, two small helpers and their reset points in
clock.ts, the lead hand-off to media sync (3 lines acrossmedia.tsandinit.ts), and four tests. It is a lone fix in the runtime's clock: no open PR touches this file to carry it.